Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Memory Leak issue in combination with Vue.js #300

Closed
bradAnderson58 opened this issue Sep 12, 2014 · 8 comments
Closed

Memory Leak issue in combination with Vue.js #300

bradAnderson58 opened this issue Sep 12, 2014 · 8 comments

Comments

@bradAnderson58
Copy link

Hello Julian,
I am having memory issues using velocity in combination with the Vue.js framework. It appears that something in velocity is still referencing destroyed elements, so they're not being properly GC. Here are demonstrations in jsfiddle:
This code has memory leak using velocity: http://jsfiddle.net/hRAn7/8/
This code has velocity call removed, GC correctly: http://jsfiddle.net/hRAn7/7/

Can you make any suggestions as to what is wrong? Any help would be appreciated.

@julianshapiro
Copy link
Owner

Yo! Thanks for filing this, Brad :)

Issue 1: Putting an animation call (to Velocity or otherwise) inside a setInterval is bad practice to begin with, but making the setInterval's timeout only 10ms is even worse practice (it's less than the typical rAF tick rate) and is guaranteed to cause memory issues.

Issue 2: I'd need to see this problem getting triggered only with Velocity. Vue has no bearing on a Velocity bug, so I would need an actual stripped down test case of this bug occurring without Vue at all.

@bradAnderson58
Copy link
Author

hey thanks for responding so quickly!
The actual applications timeout is much longer than 10ms, I was speeding it up for testing purposes, but I did read earlier today that setInterval is considered bad practice for animations. I do want the animations to loop tho, I was thinking of using setTimeout and then calling recursively, would that be better?

I slowed the timeout down to avoid guaranteed memory issues, but the problem persists. As far as using only velocity, it works fine. This code for example: http://jsfiddle.net/hRAn7/16/

Similarly the problem doesn't appear when I use only Vue and use jquery .animate instead of velocity. The problem only seems to appear with the combination of the two. I realize Vue's not your responsibility, I was just hoping you might have heard of something like this before.

@julianshapiro
Copy link
Owner

Have you tried refactoring to use Velocity's loop option instead of putting it inside a setInterval? Does that fix the issue?

@bradAnderson58
Copy link
Author

Well, I kind of misspoke when I claimed I want the animations to loop. I want the animations to execute inside a loop. Every interval, new content comes in from the left, and sort of 'bumps' everything else which eases right until it reaches the far edge, where it is removed. So there's not really a specific element where the animation loops, and the easing distance varies based on the new content.

@jws305
Copy link

jws305 commented Sep 14, 2014

I believe I found the problem. When Vue.js .$destroy() is called, it removes the elements from the DOM, however, it does not use jQuery .remove(). Because of this, the jQuery data object that velocity creates is never removed. When I call .removeData() right after .$destroy(), the problem goes away.

http://jsfiddle.net/hRAn7/18/

Would it be possible to have Velocity automatically clean up this data after an animation has completed?

@julianshapiro
Copy link
Owner

@jws305 Thanks so much.
@bradAnderson58 Would it be possible to confirm that his fix works?

Would it be possible to have Velocity automatically clean up this data after an animation has completed?
Velocity actually relies on stored today for performance purposes and for the reverse command. So, no, unfortunately. (Although I might be able to auto-detect remove events with MutationObserver.... cc @joshuasmock.) But, if we're aware of this issue, at least I can make a mention in the FAQ once @bradAnderson58 confirms.

@bradAnderson58
Copy link
Author

Confirmed. Thanks guys!

@julianshapiro
Copy link
Owner

Added to FAQ. Thank you, everyone! <3

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants